fix(ngTransclude): 'cos things are never simple #14787
Conversation
| transcludedScope.$destroy(); | ||
| function ngTranscludeCloneAttachFn(clone, transcludedScope) { | ||
| if (clone.length) { | ||
| $element.append(clone); |
There was a problem hiding this comment.
Should we also release any fallback contents for GC (e.g. fallbackContents = null) ?
There was a problem hiding this comment.
Sounds reasonable
There was a problem hiding this comment.
Actually this is more complicated. We need to compile this content into a linking function, which we will use to link and clone the fallback content, in the case that this appears inside an ng-repeat.
petebacondarwin
force-pushed
the
pr-14775
branch
from
ede0e0a to
a050d17
Compare
| compile: function ngTranscludeCompile(tElement) { | ||
|
|
||
| // Remove and cache any original content to act as a fallback | ||
| var fallbackLinkFn = $compile(tElement.contents()); |
There was a problem hiding this comment.
How common do you feel it is to use fallback content and have the user not provide their own? Wondering if it's worth calling $compile the first time the fallback content is needed rather than at compile time, but I have no idea how common this usage is to determine whether or not that'd be worth it.
There was a problem hiding this comment.
This is faster than the situation we had before before your PR, since then we were compiling AND linking the fallback content, even if it was never used.
We could optimize to lazy compile the content, but I would like to see some numbers that show it makes any real difference before doing so.
|
LGTM |
|
Thanks @gkalpak - merging |
…d correctly Closes #14787
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
fix
What is the current behavior? (You can also link to an open issue here)
broken
What is the new behavior (if this is a feature change)?
fixed (see below)
Does this PR introduce a breaking change?
nope
Please check if the PR fulfills these requirements
Other information: